Add wasm simd support #84
Conversation
|
I'll note that this is largely just a first stab, I think there's more simd acceleration in the |
|
I hope it's ok to ping you again @BurntSushi, but if you're busy and/or would prefer to not land this, just let me know! |
There was a problem hiding this comment.
Nice! This overall looks great. And hopefully this makes it easier to add other arches too in the future.
Meta question: is it possible for wasmtime itself to just use this crate? (Or implement a better memmem itself?)
So I think my main concern here is unfortunately MSRV. I don't treat MSRV bumps as semver breaking changes, but I do generally like to stay conservative for widely used crates like this one. I imagine this bumps the MSRV, although, it looks like CI never ran...?
I think the MSRV thing either means waiting to land this (bummer) or adding version sniffing to build.rs that only enables this on a new enough version of Rust. I'd be willing to support that to unlock this change. (But no new deps please. Probably copy one of @dtolnay's build.rs files.)
And we should get CI running. Not sure why this PR didn't run.
| return unsafe { Some(PrefilterFn::new(x86::sse::find)) }; | ||
| #[cfg(all(not(miri), target_arch = "wasm32", memchr_runtime_simd))] | ||
| { | ||
| if true { |
There was a problem hiding this comment.
Oh this was to prevent a lint from firing about dead code after this statement, I'll leave a comment
|
Also, with respect to benchmarks, those look great. The slowdowns are generally what I'd expect I think. At least in theory, it would be quite surprising to see improvements across the board. The benchmarks are designed to put pressure on implementation choices that trade off perf in some cases for others. |
|
Ok I think I fixed CI in 0d8dfc0 (and I see now you've approved the CI runs as well, hurray me being a new contributor!)
I'm not quite sure what you mean by this, can you elaborate?
No worries! I'm happy to fixup anything that needs working. From the CI file it looks like it's 1.41.1, and to confirm is the goal that basically this should work on wasm by default with 1.41.1? |
Yeah it might be an ill-formed question as a result of me knowing almost nothing about wasm haha. I probably have the abstraction layers totally confused to the point of it being a category error. But the question was provoked by your benchmarks. Presumably you're comparing the implementation in this crate with something. So I guess, what is that something? Where does it live? If it lives inside a runtime written in Rust (for example), then it seems plausible that it could be replaced with the code in this crate. Or maybe not?
Yeah, because I think this did work on Rust 1.41.1 with wasm? Although, maybe it's okay to have a more liberal MSRV policy there if the ecosystem is moving faster in wasm-land. Since that's the path of least resistance, let's just go that route until/if someone complains. But I think we should at least wait until it hits stable probably? If MSRV on x86 remains unchanged (which I suspect is the case) then that's great. |
|
Oh, heh, right! I should explain where the numbers came from... But nah nothing to do with Wasmtime in the sense of code in Wasmtime itself. The benchmark numbers before/after were this crate compiled with/without simd support (e.g. For MSRV, I'll leave that up to you. I don't think that this PR should affect any non-wasm targets (and if it does that's a bug I'm happy to fix). For wasm I don't think it actually affects MSRV unless you enable the simd feature, but the simd feature won't be stable until 1.54.0 (to be released tomorrow, what a coincidence I thought about this again today). If you'd like though I can implement support that forcibly disables simd support unless the rustc version is >= 1.54.0 on wasm. I'm also find waiting until tomorrow to land if you'd prefer wasm simd support hit stable :) |
|
Ooooooooo, gotya. That makes more sense. OK, so my plan here is:
And then I think it's good to go! Also, I created #87 for tracking the port of memchr to a more "generic" implementation. That would enable adding a memchr impl for WASM by "just" adding another shim that calls the generic version with a WASM vector type. |
|
For running the benchmarks in wasm it's a bit tricky because criterion out-of-the-box doesn't work on wasm32-wasi (what I was using for running in I had two local patches to get things working: basically I made things assume single-threaded and not use web things to get things running in Otherwise though my command line to execute the wasm benchmarks was: (searching in my bash history). I think I ran Also, if you're busy, I'm happy to look into the refactorings of #87 to get wasm supported with memchr as well as memmem. I skimmed the various modules and it looked like a simple enough port to the One question I had though reviewing the code. You've got an |
|
I was triggered into thinking about this again today, mind if I ping again @BurntSushi! I'm happy to help out with anything necessary to deal with this of course |
There was a problem hiding this comment.
All righty! Thanks for the ping. The thing that was holding me up here was that I wanted to run the benchmark suite locally just to be sure there weren't any regressions. So far, nothing seems to stick out to me beyond some variance. (I'll publish this run eventually.) So I think this is good to go with one small change (switch CI to stable).
Also, could you squash this all down into one commit? (Or I can do it.)
Thanks!
.github/workflows/ci.yml
Outdated
| @@ -60,6 +63,10 @@ jobs: | |||
| - build: win-gnu | |||
| os: windows-2019 | |||
| rust: stable-x86_64-gnu | |||
| - build: wasm | |||
| os: ubuntu-18.04 | |||
| rust: beta-x86_64-gnu # waiting for wasm simd to hit stable | |||
There was a problem hiding this comment.
I think this can be switched to stable now?
|
Sure thing, done now! (squashed, switched to stable, and updated Wasmtime as well) |
This commit adds simd acceleration support to the `memmem` module. This is added with the freshly-stabilized support from rust-lang/rust#86204. This mostly just cribs off the generic simd support for 128-bit types built for sse, copying bits and pieces of code here and there. Some refactoring happened internally to help reduce duplication where possible. I ran some initial benchmarks with the `memmem/krate/*` regex and a hacked up single-threaded version of criterion. Some [initial comparisons][compare] using Wasmtime as a runtime do indeed show a lot of improvements, but there are indeed some slowdowns as well. [compare]: https://gist.github.com/alexcrichton/6a72e682e7b6d505ade605359fbe3f2d
|
Urk. Sorry, I flubbed the benchmarks. Redoing them now. Once that's done, I'll put out a |
|
Oh no rush! I had a branch somewhere at some point that did most of the "other half" of this crate in porting it to the same style as what's here. I can try to clean that up to post it here so wasm gets a benefit from all the algorithms here instead of just some of them |
|
Yeah that would be great! Thank you. :-) |
This is a long overdue benchmark of the WASM implementation that landed in #84. I tried to do this a few months ago, but I mucked it up and never got back around to it. (This is also why I haven't put out a new release with the WASM implementation, since I wanted to do a sanity benchmark run first.)
This is a long overdue benchmark of the WASM implementation that landed in #84. I tried to do this a few months ago, but I mucked it up and never got back around to it. (This is also why I haven't put out a new release with the WASM implementation, since I wanted to do a sanity benchmark run first.)
|
@alexcrichton So sorry about the delay here, but I've finally published this patch in |
|
No worries at all, and thanks again @BurntSushi! |
This commit adds simd acceleration support to the
memmemmodule. Thisis added with the freshly-stabilized support from rust-lang/rust#86204.
This mostly just cribs off the generic simd support for 128-bit types
built for sse, copying bits and pieces of code here and there. Some
refactoring happened internally to help reduce duplication where
possible.
I ran some initial benchmarks with the
memmem/krate/*regex and ahacked up single-threaded version of criterion. Some initial
comparisons using Wasmtime as a runtime do indeed show a lot
of improvements, but there are indeed some slowdowns as well.